Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(collection): add findSingle #1166

Merged
merged 10 commits into from
Sep 12, 2021
Merged

Conversation

NotFounds
Copy link
Contributor

Implement single for collection module.
(ref: #1065)

The type definition is based on this.

ref.
#1065 (comment)
#978

@CLAassistant
Copy link

CLAassistant commented Aug 26, 2021

CLA assistant check
All committers have signed the CLA.

@NotFounds NotFounds force-pushed the add-collection-single branch from e345002 to 820e27b Compare August 26, 2021 07:22
Copy link
Contributor

@LionC LionC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good to me! Includes all the important easily overlooked details, well written tests, docs...

Thank you very much :-) I hope @kt3k agrees^^

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NotFounds Thank you for your contribution.

I generally agree with this addition. Left a few comments.

### single

Returns the only element in the given collection matching the given predicate.
Returns undefined if there is none.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should mention more explicitly about returning undefined when there are multiple matches

and it might be good to have another example code block which has multiple matches

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review.
I added an example and description for returning undefined cases in this commit (7472844 6c74b61).

Would you please confirm that this explanation is enough?

@kt3k
Copy link
Member

kt3k commented Aug 26, 2021

I'm a little concerned about the name of the function. It sounds like an adjective to me. (We discussed this topic in #1098 )

Does anyone have any other suggestion for the name of this?

@LionC
Copy link
Contributor

LionC commented Aug 26, 2021

I'm a little concerned about the name of the function. It sounds like an adjective to me. (We discussed this topic in #1098 )

Does anyone have any other suggestion for the name of this?

Kotlin calls it single, hence the initial name . I personally see why, it reads well when used in context. Maybe getSingle is an alternative? Or only? getOnly? getOnlyElement? onlyElement? :-S

@NotFounds NotFounds marked this pull request as ready for review August 27, 2021 08:31
### single

Returns the only element in the given collection matching the given predicate.
Returns undefined if there is none or multiple matches for the predicate.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternative:

Returns the element if and only if a single matching element exists to the given condition; otherwise, it returns undefined.

Copy link
Contributor

@LionC LionC Aug 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to the given condition" does not make sense to me (not a native speaker though). Maybe slightly modify to

Returns an element if and only if that element is the only one matching the given condition.

Returns `undefined` otherwise.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for comment! Fixed it! 1fa2dbb

@NotFounds NotFounds requested a review from kt3k August 27, 2021 09:13
@LionC LionC mentioned this pull request Aug 27, 2021
44 tasks
@LionC
Copy link
Contributor

LionC commented Aug 30, 2021

@kt3k I think this is approaching mergability - what should we do about the name?

@kt3k
Copy link
Member

kt3k commented Aug 31, 2021

@LionC

what should we do about the name?

I started feeling the name is ok as is because single seems having noun usage.

Does anyone have any opinion about this? cc @timreichen

@timreichen
Copy link
Contributor

@kt3k
I think single is not a good name, firstly because it is not a verb and secondly because it is not descriptive at all on what it does.
Since we have a distinct and distinctBy, how about findDistinct?

@LionC
Copy link
Contributor

LionC commented Aug 31, 2021

@kt3k
I think single is not a good name, firstly because it is not a verb and secondly because it is not descriptive at all on what it does.
Since we have a distinct and distinctBy, how about findDistinct?

If you read

const file = findDistinct(Deno.args)

do you feel like you would know what's going on here?

In my (non-native speaker) mind the word distinct needs other values to make sense and does not really make sense for a one-element array.

@kt3k
Copy link
Member

kt3k commented Aug 31, 2021

@timreichen Thank you for the input, but findDistinct sounds a little strange to me..

How about findSingle?

@timreichen
Copy link
Contributor

timreichen commented Aug 31, 2021

@kt3k This will make deno std attractive for people that do partnership search:) I don't know, but better than just single still not really expressive imo.
@LionC

const file = findDistinct(Deno.args)

tbh not so much since the default predicate is set to (_) => true (which I don't know is a good idea if we use find as the verb as in js find and findIndex have a mandatory predicates?).

In that sense I think having this default predicate changes the meaning of the function to Deno.args.length === 1 ? Deno.args[0] : undefined where with a custom predicate it is about finding a distinct value in an array.

const value = findDistinct(["f", "o", "o"], (char) => char === "o") // undefined
const value = findDistinct(["f", "o", "o"], (char) => char === "f") // "f"

btw:
not sure about this check:

if (match !== undefined) return undefined;`

Hypothetically this will not short circuit for undefined values:

const array = new Array(1000).fill(undefined)
const value = findDistinct(array, (i) => i === undefined); // predicate called 1000x

Maybe a flag should be used instead?

let match: T | undefined = undefined;
let found = false
for (const element of array) {
  if (predicate(element)) {
    if (found) return undefined;
    found = true
    match = element;
  }
}
return match;

@LionC
Copy link
Contributor

LionC commented Aug 31, 2021

@timreichen

@kt3k This will make deno std attractive for people that do partnership search:)

Adoption boost spotted^^

tbh not so much since the default predicate is set to (_) => true (which I don't know is a good idea if we use find as the verb as in js find and findIndex have a mandatory predicates?).

Yeah that is what I meant - find does not fit that well but it is an essential part of the usage/signature (stolen from kotlin again).

Hypothetically this will not short circuit for undefined values:

const array = new Array(1000).fill(undefined)

const value = findDistinct(array, (i) => i === undefined); // predicate called 1000x

Oh yes, we should change that @NotFounds . Good catch!

@NotFounds
Copy link
Contributor Author

@timreichen
Thank you for your review and suggestion.
I didn't take that case into consideration...

Fixed it! 8864057

@LionC
Copy link
Contributor

LionC commented Sep 1, 2021

Should be mergeable now @kt3k

@kt3k
Copy link
Member

kt3k commented Sep 1, 2021

Can we settle on the name 'single'?

@NotFounds Do you have any preference on naming?

@NotFounds
Copy link
Contributor Author

(I'm not a native speaker)
I'm comfortable with the use of "single".
And especially when the behavior is the same as the method used in other languages.

But... if a verb is preferred, the suggested "findSingle" is the best fit for me.

@lucacasonato
Copy link
Member

lucacasonato commented Sep 2, 2021

findSingle?

@LionC
Copy link
Contributor

LionC commented Sep 3, 2021

Sounds like single and findSingle are the options. I would vote for single because it is short :-)

@kt3k I think we need your/some core call here.

@kt3k
Copy link
Member

kt3k commented Sep 8, 2021

Note

NotFounds: single or findSingle
Leon: single
Luca: findSingle
Tim: findDistinct
Grian: single or findSingle
Yoshi: single or findSingle

(new votes are welcome!)

@timreichen
Copy link
Contributor

@kt3k
I would join the findSingle crew if we removed the default selector (reasons why described above)

@LionC
Copy link
Contributor

LionC commented Sep 8, 2021

I dont think we should remove functionality for a name, but find a name for functionality

@timreichen
Copy link
Contributor

timreichen commented Sep 10, 2021

I think findSingle or findDistinct describes the algorithm of the function pretty well. So I argue that it is not removing functionality but removing a bad default that changes the intent of the function. Consider findSingle([]), findSingle([undefined]) and findSingle([undefined, undefined]): The return values will be undefined for all of them with the current default predicate, making the intended usage of this function questionable. That default predicate might make sense in kotlin because it throws an exception if it is not successful. Because this implementation returns undefined on failure that predicate is not really useful imo.

To make sure a single value exists in an array array.length === 1 should be used instead of that function anyhow, so why not generalize it by removing the predicate therefore solving the naming problem too?

@LionC
Copy link
Contributor

LionC commented Sep 12, 2021

I think findSingle or findDistinct describes the algorithm of the function pretty well. So I argue that it is not removing functionality but removing a bad default that changes the intent of the function. Consider findSingle([]), findSingle([undefined]) and findSingle([undefined, undefined]): The return values will be undefined for all of them with the current default predicate, making the intended usage of this function questionable. That default predicate might make sense in kotlin because it throws an exception if it is not successful. Because this implementation returns undefined on failure that predicate is not really useful imo.

Typings make that undefined case pretty clear I think - I can not come up with any bugs that could cause, but maybe I am missing them. It is really similar to an index access returning undefined vs that index's value actually being undefined.

To make sure a single value exists in an array array.length === 1 should be used instead of that function anyhow, so why not generalize it by removing the predicate therefore solving the naming problem too?

I mean you can always write more code instead of using something that encapsulates it, sure. Saying "should" here is a stretch to me though - why "should" someone not use that function if they want to? It might not be your style, but it might be theirs and it is hard to say someone objectively "should" not do it.

Either way, I think I would be fine with findSingle the way it is right now if that is what it takes to get this merged^^ So @kt3k you can consider me in the "single or findSingle" camp as well :-)

@kt3k
Copy link
Member

kt3k commented Sep 12, 2021

Let's settle on the name findSingle because everybody is at least OK with it.

Let's continue the discussion of the default selector in another issue.

I'm merging this for now as this one is enough discussed and looks having enough quality now. Thank you for your efforts!

@kt3k kt3k changed the title Add collection single feat(collection): add findSingle Sep 12, 2021
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NotFounds LGTM. Thank you for your contribution!

@kt3k kt3k merged commit 361c3e4 into denoland:main Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants